Use yaml.safe_load() and yaml.safe_dump() #58
Conversation
Gentoo has disabled yaml.load() in response to CVE-2017-18342, see https://bugs.gentoo.org/659348 for more details. This results in clay being unusabel on Gentoo systems, as loading the settings YAML causes the application to fail. This patch changes the yaml.load() calls to yaml.safe_load(), which avoids the security issue and has been left enabled by Gentoo. Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
yaml.dump() is capable of producing files that cannot be read by yaml.load(), which would result in the application being unable to load the settings file. I haven't actually seen this happen, I just noticed safe_dump() when doing the safe_load() change. Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
|
Hi Palmer, Thanks for your contributions to Clay, I appreciate the time and effort you spent on it. However, most of the pull requests address bugs that already have been fixed but haven't landed in the master branch yet due to issues within the project (described fully in #57, but boils down to project owner being gone and me not knowing what to do). This one, for instance, fixes #55 which already includes a solution and another fixes #38 where the bug reporter, @agg23, also contributed a patch. I can't really blame you for this, it is both confusing and my entirely my fault, but please make sure to either base your work on the development branch (called 'porcelain') or contribute to my up-to-date soft hard fork at ValentijnvdBeek/clay. Anyways, to the pull request itself, the behaviour of Gentoo is kind ridiculous in the sense that we aren't loading any YaML from the internet at all so the security vulnerability doesn't affect us. Suffice to say that the moment an attacker can access and freely modify a file, all is already lost. The two commits are related however since safe_dump and safe_load address the same thing, mainly which subset of Python it can process. On later on versions of PyYaML using the dump procedure will give a warning. Porcelain carries a fix for the load and my fork has both fixed. Anyways, long story short, good work on the commits. They look good and mergeable accept for the fact that they, by happenstance, already have been fixed. I'll look at the others in short order but do keep in mind that I am on vacation in France for the upcoming three weeks so it'll take me some time to get back to you. Cheers, Valentijn |
These two changes are only related in that I found them at the same time. The change to yaml.load() fixes a crash on Gentoo-based systems. The change to yaml.dump() doesn't fix an error, I just stumbled across it. More information is in the patches.